Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Watermark exception #3019

Merged
merged 11 commits into from
Nov 3, 2023
Merged

Watermark exception #3019

merged 11 commits into from
Nov 3, 2023

Conversation

akhilsrivatsa
Copy link
Contributor

Fixes

Fixes #1234 by @obulat

Description

Testing Instructions

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin 
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@akhilsrivatsa akhilsrivatsa requested a review from a team as a code owner September 12, 2023 18:28
@openverse-bot openverse-bot added 🟩 priority: low Low priority and doesn't need to be rushed 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository 🧱 stack: api Related to the Django API labels Sep 12, 2023
@akhilsrivatsa
Copy link
Contributor Author

@obulat @krysal

obulat
obulat reviewed Sep 18, 2023
View reviewed changes
Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution, @akhilsrivatsa, it's a great start. We would also probably need a test for this change.

To help the reviewers, please fill in the PR description and the testing instructions. You can see what we usually write there in other PRs.

obulat
obulat reviewed Sep 18, 2023
View reviewed changes
api/api/utils/watermark.py Outdated Show resolved Hide resolved
@obulat
Copy link
Contributor

obulat commented Sep 19, 2023

@akhilsrivatsa, the CI check logs show the following errors that need to be fixed:

api/api/utils/watermark.py:27:34: F821 Undefined name `APIException`
api/api/utils/watermark.py:28:19: F821 Undefined name `status`
api/api/utils/watermark.py:29:89: E501 Line too long (89 > 88 characters)
Found 3 errors.

You can also lint the code locally using just lint.

@akhilsrivatsa
Copy link
Contributor Author

@obulat I'm working on fixing the bugs

@krysal krysal marked this pull request as draft September 19, 2023 15:32
@krysal
Copy link
Member

krysal commented Sep 19, 2023

@akhilsrivatsa I converted your PR to a draft by the moment, let us know when it is ready!

@akhilsrivatsa
Copy link
Contributor Author

@obulat I've fixed the issues. There's some check that's still failing in CI + CD/ lint files. I'm not sure why.

obulat
obulat requested changes Sep 24, 2023
View reviewed changes
Copy link
Contributor

@obulat obulat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another exception that should be handled here, UnidentifiedImageError. It is often caused by SVG files because SVG is not supported by Pillow.

Here's the patch of changes that can be added to fix it:

diff --git a/api/api/utils/watermark.py b/api/api/utils/watermark.py
--- a/api/api/utils/watermark.py	(revision d866e262e6a432fc0e54fcf92fce71733b44d6ad)
+++ b/api/api/utils/watermark.py	(date 1695537703291)
@@ -7,7 +7,8 @@
 from django.conf import settings
 
 import requests
-from PIL import Image, ImageDraw, ImageFont
+from PIL import Image, ImageDraw, ImageFont, UnidentifiedImageError
+from requests import RequestException
 from sentry_sdk import capture_exception
 
 from rest_framework import status
@@ -183,10 +184,10 @@
         response.raise_for_status()
         img_bytes = BytesIO(response.content)
         img = Image.open(img_bytes)
-    except requests.exceptions.RequestException as e:
+    except (RequestException, UnidentifiedImageError) as e:
         capture_exception(e)
         logger.error(f"Error loading image data: {e}")
-        raise UpstreamWatermarkException(f"Error: {e}")
+        raise UpstreamWatermarkException(f"{e}")
 
     return img, img.getexif()

We also should add tests, at least for the 404 response and for images that raise the UnidentifiedImageError. Here's a patch of tests that could be used:

diff --git a/api/test/unit/views/test_image_views.py b/api/test/unit/views/test_image_views.py
--- a/api/test/unit/views/test_image_views.py	(revision d866e262e6a432fc0e54fcf92fce71733b44d6ad)
+++ b/api/test/unit/views/test_image_views.py	(date 1695537748677)
@@ -2,6 +2,8 @@
 from collections.abc import Callable
 from dataclasses import dataclass
 from pathlib import Path
+from PIL import UnidentifiedImageError
+
 from test.factory.models.image import ImageFactory
 from unittest.mock import ANY, patch
 
@@ -79,3 +81,28 @@
         thumb_call.return_value = mock_response
         api_client.get(f"/v1/images/{image.identifier}/thumb/")
     thumb_call.assert_called_once_with(ANY, image, expected_thumb_url)
+
+
+@pytest.mark.django_db
+def test_watermark_raises_424_for_invalid_image(api_client):
+    image = ImageFactory.create()
+    expected_error_message = "cannot identify image file <_io.BytesIO object at 0xffff86d8fec0>"
+
+    with patch("PIL.Image.open") as mock_open:
+        mock_open.side_effect = UnidentifiedImageError(expected_error_message)
+        res = api_client.get(f"/v1/images/{image.identifier}/watermark/")
+    assert res.status_code == 424
+    assert res.data["detail"] == expected_error_message
+
+@pytest.mark.django_db
+def test_watermark_raises_424_for_404_image(api_client):
+    image = ImageFactory.create()
+
+    with patch("requests.get") as mock_get:
+        mock_get.return_value = Response()
+        mock_get.return_value.status_code = 404
+        mock_get.return_value.url = image.url
+        mock_get.return_value.reason = "Not Found"
+        res = api_client.get(f"/v1/images/{image.identifier}/watermark/")
+    assert res.status_code == 424
+    assert res.data["detail"] == f"404 Client Error: Not Found for url: {image.url}"

To test the changes locally, you can also update the sample_image.csv to replace the URLs for the first two images, as in this diff:

diff --git a/sample_data/sample_image.csv b/sample_data/sample_image.csv
--- a/sample_data/sample_image.csv	(revision d866e262e6a432fc0e54fcf92fce71733b44d6ad)
+++ b/sample_data/sample_image.csv	(date 1695535546283)
@@ -1,6 +1,6 @@
 identifier,created_on,updated_on,ingestion_type,provider,source,foreign_identifier,foreign_landing_url,url,thumbnail,width,height,filesize,license,license_version,creator,creator_url,title,meta_data,tags,watermarked,last_synced_with_source,removed_from_source,filetype,category,standardized_popularity
-0e3315c5-3328-4a99-80ab-567ac32f685f,2022-12-21 17:29:54.000000+00,2022-12-21 17:29:54.000000+00,provider_api,flickr,flickr,51745822704,https://www.flickr.com/photos/54633257@N04/51745822704,https://live.staticflickr.com/65535/51745822704_ae97226e20_b.jpg,https://live.staticflickr.com/65535/51745822704_ae97226e20_m.jpg,433,1024,97150,by-nc-sa,2.0,mexicofist3,https://www.flickr.com/photos/54633257@N04,my dress is not very short,"{""views"": ""5991"", ""pub_date"": ""1639443671"", ""date_taken"": ""2021-07-20 18:19:25"", ""description"": ""dressed"", ""license_url"": ""https://creativecommons.org/licenses/by-nc-sa/2.0/"", ""raw_license_url"": null}",,f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
-b840de61-fb9d-4ec5-9572-8d778875869f,2022-03-08 14:32:25.000000+00,2022-03-08 14:32:25.000000+00,provider_api,flickr,flickr,51745349583,https://www.flickr.com/photos/129684398@N07/51745349583,https://live.staticflickr.com/65535/51745349583_cebf1fa6e0_b.jpg,https://live.staticflickr.com/65535/51745349583_cebf1fa6e0_m.jpg,1024,713,171956,by-nc,2.0,clairetresse,https://www.flickr.com/photos/129684398@N07,Barranco de las bodegas,"{""views"": ""4156"", ""pub_date"": ""1639440673"", ""date_taken"": ""2021-10-28 10:54:43"", ""description"": ""Descente dans un des Barranco, descente prudente parce que ça glisse. L’érosion des sols est extrêmement puissante, les reliefs sont sculptés par les forces de la nature telles que le vente les pluies . Descent in one of the Barranco, careful descent because it slips. The erosion of the soil is extremely powerful, the reliefs are sculpted by the forces of nature such as the rains."", ""license_url"": ""https://creativecommons.org/licenses/by-nc/2.0/"", ""raw_license_url"": null}","[{""name"": ""bardenas"", ""provider"": ""flickr""}, {""name"": ""barranco"", ""provider"": ""flickr""}, {""name"": ""desert"", ""provider"": ""flickr""}, {""name"": ""espagne"", ""provider"": ""flickr""}, {""name"": ""glaise"", ""provider"": ""flickr""}, {""name"": ""navarre"", ""provider"": ""flickr""}, {""name"": ""pyrennees"", ""provider"": ""flickr""}, {""name"": ""ravin"", ""provider"": ""flickr""}, {""name"": ""roches"", ""provider"": ""flickr""}, {""name"": ""water"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
+0e3315c5-3328-4a99-80ab-567ac32f685f,2022-12-21 17:29:54.000000+00,2022-12-21 17:29:54.000000+00,provider_api,flickr,flickr,51745822704,https://www.flickr.com/photos/54633257@N04/51745822704,https://live.staticflickr.com/2914/14425817605_87df10788d_b.jpg,https://live.staticflickr.com/65535/51745822704_ae97226e20_m.jpg,433,1024,97150,by-nc-sa,2.0,mexicofist3,https://www.flickr.com/photos/54633257@N04,my dress is not very short,"{""views"": ""5991"", ""pub_date"": ""1639443671"", ""date_taken"": ""2021-07-20 18:19:25"", ""description"": ""dressed"", ""license_url"": ""https://creativecommons.org/licenses/by-nc-sa/2.0/"", ""raw_license_url"": null}",,f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
+b840de61-fb9d-4ec5-9572-8d778875869f,2022-03-08 14:32:25.000000+00,2022-03-08 14:32:25.000000+00,provider_api,flickr,flickr,51745349583,https://www.flickr.com/photos/129684398@N07/51745349583,https://raw.githubusercontent.com/WordPress/openverse/main/frontend/src/assets/svg/raw/caret-down.svg,https://live.staticflickr.com/65535/51745349583_cebf1fa6e0_m.jpg,1024,713,171956,by-nc,2.0,clairetresse,https://www.flickr.com/photos/129684398@N07,Barranco de las bodegas,"{""views"": ""4156"", ""pub_date"": ""1639440673"", ""date_taken"": ""2021-10-28 10:54:43"", ""description"": ""Descente dans un des Barranco, descente prudente parce que ça glisse. L’érosion des sols est extrêmement puissante, les reliefs sont sculptés par les forces de la nature telles que le vente les pluies . Descent in one of the Barranco, careful descent because it slips. The erosion of the soil is extremely powerful, the reliefs are sculpted by the forces of nature such as the rains."", ""license_url"": ""https://creativecommons.org/licenses/by-nc/2.0/"", ""raw_license_url"": null}","[{""name"": ""bardenas"", ""provider"": ""flickr""}, {""name"": ""barranco"", ""provider"": ""flickr""}, {""name"": ""desert"", ""provider"": ""flickr""}, {""name"": ""espagne"", ""provider"": ""flickr""}, {""name"": ""glaise"", ""provider"": ""flickr""}, {""name"": ""navarre"", ""provider"": ""flickr""}, {""name"": ""pyrennees"", ""provider"": ""flickr""}, {""name"": ""ravin"", ""provider"": ""flickr""}, {""name"": ""roches"", ""provider"": ""flickr""}, {""name"": ""water"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
 aeba0547-61da-42ee-b561-27c8fc817d5a,2022-07-16 05:51:03.000000+00,2022-07-16 05:51:03.000000+00,provider_api,flickr,flickr,51745788239,https://www.flickr.com/photos/88123769@N02/51745788239,https://live.staticflickr.com/65535/51745788239_b645ce02fe_b.jpg,https://live.staticflickr.com/65535/51745788239_b645ce02fe_m.jpg,1024,602,281512,pdm,1.0,Bernard Spragg,https://www.flickr.com/photos/88123769@N02,Alone on the prairie.,"{""views"": ""1779"", ""pub_date"": ""1639441826"", ""date_taken"": ""2017-09-26 11:39:16"", ""description"": ""The Canadian Prairies (usually referred to as simply the Prairies in Canada) is a region in Western Canada. It includes the Canadian portion of the Great Plains and the Prairie Provinces, namely Alberta, Saskatchewan, and Manitoba.These provinces are partially covered by grasslands, plains, and lowlands, mostly in the southern regions."", ""license_url"": ""https://creativecommons.org/publicdomain/mark/1.0/"", ""raw_license_url"": null}","[{""name"": ""alberta"", ""provider"": ""flickr""}, {""name"": ""alone"", ""provider"": ""flickr""}, {""name"": ""canada"", ""provider"": ""flickr""}, {""name"": ""evening"", ""provider"": ""flickr""}, {""name"": ""house"", ""provider"": ""flickr""}, {""name"": ""landscape"", ""provider"": ""flickr""}, {""name"": ""lumixfz1000"", ""provider"": ""flickr""}, {""name"": ""old"", ""provider"": ""flickr""}, {""name"": ""outside"", ""provider"": ""flickr""}, {""name"": ""prairie"", ""provider"": ""flickr""}, {""name"": ""rural"", ""provider"": ""flickr""}, {""name"": ""scenery"", ""provider"": ""flickr""}, {""name"": ""sky"", ""provider"": ""flickr""}, {""name"": ""travel"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,
 3c98150c-51a8-4175-a47f-acef10e784f7,2022-06-10 09:14:13.000000+00,2022-06-10 09:14:13.000000+00,provider_api,flickr,flickr,51747927224,https://www.flickr.com/photos/151325871@N07/51747927224,https://live.staticflickr.com/65535/51747927224_3ca7ac2e93.jpg,https://live.staticflickr.com/65535/51747927224_3ca7ac2e93_m.jpg,318,500,53633,cc0,1.0,lyndawaybi3,https://www.flickr.com/photos/151325871@N07,Naughty Little Elf,"{""views"": ""1342"", ""pub_date"": ""1639526583"", ""date_taken"": ""2021-12-14 16:02:55"", ""license_url"": ""https://creativecommons.org/publicdomain/zero/1.0/""}","[{""name"": ""babe"", ""provider"": ""flickr""}, {""name"": ""bi"", ""provider"": ""flickr""}, {""name"": ""brunette"", ""provider"": ""flickr""}, {""name"": ""chick"", ""provider"": ""flickr""}, {""name"": ""christmas"", ""provider"": ""flickr""}, {""name"": ""dress"", ""provider"": ""flickr""}, {""name"": ""elf"", ""provider"": ""flickr""}, {""name"": ""great"", ""provider"": ""flickr""}, {""name"": ""hot"", ""provider"": ""flickr""}, {""name"": ""hotwife"", ""provider"": ""flickr""}, {""name"": ""leggings"", ""provider"": ""flickr""}, {""name"": ""legs"", ""provider"": ""flickr""}, {""name"": ""lynda"", ""provider"": ""flickr""}, {""name"": ""married"", ""provider"": ""flickr""}, {""name"": ""milf"", ""provider"": ""flickr""}, {""name"": ""mini"", ""provider"": ""flickr""}, {""name"": ""mom"", ""provider"": ""flickr""}, {""name"": ""nylons"", ""provider"": ""flickr""}, {""name"": ""panyhose"", ""provider"": ""flickr""}, {""name"": ""season"", ""provider"": ""flickr""}, {""name"": ""sexy"", ""provider"": ""flickr""}, {""name"": ""short"", ""provider"": ""flickr""}, {""name"": ""skirt"", ""provider"": ""flickr""}, {""name"": ""stockings"", ""provider"": ""flickr""}, {""name"": ""sweater"", ""provider"": ""flickr""}, {""name"": ""wife"", ""provider"": ""flickr""}, {""name"": ""young"", ""provider"": ""flickr""}]",f,2021-12-15 22:19:02.971943+00,f,jpg,photograph,
 cdbd3bf6-1745-45bb-b399-61ee149cd58a,2022-12-28 15:41:34.000000+00,2022-12-28 15:41:34.000000+00,provider_api,flickr,flickr,51745389858,https://www.flickr.com/photos/126744325@N07/51745389858,https://live.staticflickr.com/65535/51745389858_c10358e1a3_b.jpg,https://live.staticflickr.com/65535/51745389858_c10358e1a3_m.jpg,1024,683,157497,by,2.0,Kristoffer Trolle,https://www.flickr.com/photos/126744325@N07,Train area in Copenhagen South / Tog område i Syd København,"{""views"": ""1337"", ""pub_date"": ""1639441947"", ""date_taken"": ""2021-07-14 23:49:46"", ""description"": ""This old train area in Copenhagen South will soon be transformed into a residential area. I love to go there and take photos. I used a Tiffen Black Pro Mist 1/4 filter for this photo, it gives that diffused highlights look, read more about it on my blog here . The photo is Creative Commons license: Use it for free. Keywords: train, tog, DSB, område, syd, København, south, Copenhagen, Danmark, Denmark, Fujifilm X-H1, Fujifilm XF 35mm f2 R WR, Tiffen Black Pro-Mist 1/4 filter"", ""license_url"": ""https://creativecommons.org/licenses/by/2.0/"", ""raw_license_url"": null}","[{""name"": ""copenhagen"", ""provider"": ""flickr""}, {""name"": ""danmark"", ""provider"": ""flickr""}, {""name"": ""denmark"", ""provider"": ""flickr""}, {""name"": ""dsb"", ""provider"": ""flickr""}, {""name"": ""fujifilmxf35mmf2rwr"", ""provider"": ""flickr""}, {""name"": ""fujifilmxh1"", ""provider"": ""flickr""}, {""name"": ""københavn"", ""provider"": ""flickr""}, {""name"": ""område"", ""provider"": ""flickr""}, {""name"": ""south"", ""provider"": ""flickr""}, {""name"": ""syd"", ""provider"": ""flickr""}, {""name"": ""tiffenblackpromist14filter"", ""provider"": ""flickr""}, {""name"": ""tog"", ""provider"": ""flickr""}, {""name"": ""train"", ""provider"": ""flickr""}]",f,2021-12-15 20:49:19.976193+00,f,jpg,photograph,

You should run just recreate, just init after these changes to make sure the new sample data is used.
Then, you would be able to see fixed responses for the following urls:
http://localhost:50280/v1/images/0e3315c5-3328-4a99-80ab-567ac32f685f/watermark/
http://localhost:50280/v1/images/b840de61-fb9d-4ec5-9572-8d778875869f/watermark/

@akhilsrivatsa
Copy link
Contributor Author

@obulat I'll work on fixing the UnidentifiedImageError.

@obulat
Copy link
Contributor

obulat commented Oct 6, 2023

@akhilsrivatsa, do you plan to work on this PR? It's Okay if you don't - but please let us know if so, or if there's anything we can help you with to get this PR finished.

@obulat obulat force-pushed the watermark-exception branch from d866e26 to 5d7b3bb Compare October 10, 2023 14:08
krysal
krysal previously requested changes Oct 19, 2023
View reviewed changes
Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @akhilsrivatsa! This is looking quite close to a complete solution. Can you still work on this? I left some comments with suggestions.

api/api/utils/watermark.py Outdated Show resolved Hide resolved
api/test/unit/views/test_image_views.py Show resolved Hide resolved
@obulat obulat force-pushed the watermark-exception branch from 16c920e to f1ad8fa Compare October 25, 2023 15:27
@obulat obulat force-pushed the watermark-exception branch from f1ad8fa to 21b38e8 Compare October 27, 2023 11:20
@obulat obulat requested review from obulat and krysal October 27, 2023 11:27
@obulat obulat marked this pull request as ready for review October 27, 2023 11:27
@obulat obulat requested a review from dhruvkb November 1, 2023 15:39
dhruvkb
dhruvkb approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM; thanks for the contribution @akhilsrivatsa!

I would prefer that exceptions like UpstreamWatermarkException be defined in a standalone file like exceptions.py but this works and resolves a long-standing issue.

@dhruvkb dhruvkb dismissed krysal’s stale review November 3, 2023 17:23

Changes have been implemented.

@dhruvkb dhruvkb merged commit b2a3970 into WordPress:main Nov 3, 2023
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhruvkb dhruvkb dhruvkb approved these changes

@obulat obulat obulat approved these changes

@stacimc stacimc Awaiting requested review from stacimc stacimc is a code owner automatically assigned from WordPress/openverse-api

@krysal krysal Awaiting requested review from krysal

Assignees
No one assigned
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: api Related to the Django API
Projects
Openverse PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unhandled exception when reading image in watermark route
5 participants
@akhilsrivatsa @obulat @krysal @dhruvkb @openverse-bot